Pin six more guards to their property, not their caller count (#1078) by OffgridwithJD · Pull Request #1081 · commandprompt/pgcolumnar

OffgridwithJD · 2026-09-16T17:12:29Z

#1078 repaired two arms in native_fetch_projection.sh that compared a whole-file grep -c against a literal. A sweep of test/ found twelve sites of that shape. These are six of the remaining ten.

The repair differs per arm, because the failure direction does

That is the part worth reading, and it is why this is not one sed:

arm	fails how	repaired as
the entry key	both directions	self-referential, `keyed N of N`
the cid reject	noise only	scoped to the function that must hold it
the geometry	blind to 3 of 4 fields	membership over all four compared fields
the discard	a proxy for where	the two functions named
rank, valOffset	noise only	scoped to `pgcolumnar_fetch_row`

Two of the eight I was assigned are correct and are left alone

A pinned count is right where the count is the property:

decode_interrupts.sh's ^#define COLUMNAR_DECODE_INTERRUPT(i) — a second #define is a redefinition, a real defect.
native_saop_pushdown.sh's premise: plain-key special flags have one reject expression — load-bearing for the awk range on the next line, which would silently concatenate two expressions into one guard string and then pass every membership test on the union.

Classifying those two took longer than fixing the six, and getting it wrong would have been a widening dressed as a fix.

Measured

Every mutation compiles, so the suite rebuilds and runs end to end — the harness refuses a source/.so mismatch, correctly, and there is no shortcut past it.

case                        OLD arms            NEW arms
unkeyed group lookup        key=1     PASS      RED  keyed 1 of 2
second keyed lookup         key=2     RED       24 passed
rowCount dropped            geom=1    PASS      RED  rowCount
executor-end discard gone   discard=1 RED       RED  names the function
third discard call          discard=3 RED       24 passed
rank replaced by a walk     rank=0    RED       RED  rank prefix

The two OLD ... PASS rows are the case for this change. An unkeyed group lookup and a dropped geometry field both leave the old arms green. The two OLD RED / NEW passed rows are what fired on #1077 and cost a correct PR a red.

My first mutation matrix was wrong and I nearly shipped it

It reported the second-keyed-lookup case reddening an unrelated natts arm. Run alone, that case is 24/24 with all four geometry arms green.

The harness asserted each mutation applied and never asserted it was restored, so one case was measuring two mutations at once. Re-run with an md5 restore assertion per case and the contamination is gone.

A mutation harness that does not check its own restore produces exactly the false finding I would have filed against someone else.

Five majors, both suites, own `make clean` each

PG15  PG16  PG17  PG18  PG19
native_fetch_cache.sh:    PASSED  x5
native_fetch_position.sh: PASSED  x5

No ledger change

native_fetch_cache and native_fetch_position have zero rows in the ledger, so they are two of the 249 uncovered suites and no check name here is a ledger key. Checked on this tree rather than inherited from #1078.

What remains of the twelve

Four sites, none in these two files: the two #1078 fixed, and the two above that are correct as written. native_fetch_cache and native_fetch_position now have none outstanding.

🤖 Generated with Claude Code

https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs

…dprompt#1078) commandprompt#1078 repaired two arms in native_fetch_projection.sh that compared a whole-file `grep -c` against a literal. A sweep of test/ found TWELVE sites of that shape. These are six of the remaining ten. THE REPAIR DIFFERS PER ARM BECAUSE THE FAILURE DIRECTION DOES, which is the part worth reading: the entry key both directions -> self-referential, keyed N of N the cid reject noise only -> scoped to the function that must hold it the geometry blind to 3 of 4 -> membership over all four compared fields the discard proxy for "where" -> the two functions named rank, valOffset noise only -> scoped to pgcolumnar_fetch_row TWO OF THE EIGHT I WAS ASSIGNED ARE CORRECT AND ARE LEFT ALONE. A pinned count is right where the count IS the property: `^#define COLUMNAR_DECODE_INTERRUPT(i)` appearing twice would be a redefinition, and native_saop_pushdown's premise is load-bearing for an `awk` range that would silently concatenate two expressions into one `guard` string. Classifying them took longer than fixing the six. MEASURED. Every mutation compiles, so the suite rebuilds and runs end to end -- the harness refuses a source/.so mismatch, correctly, and there is no shortcut: case OLD arms NEW arms unkeyed group lookup key=1 PASS RED keyed 1 of 2 second keyed lookup key=2 RED 24 passed rowCount dropped geom=1 PASS RED rowCount executor-end discard gone discard=1 RED RED names the function third discard call discard=3 RED 24 passed rank replaced by a walk rank=0 RED RED rank prefix The two OLD-PASS rows are the case for the change: an unkeyed lookup and a dropped geometry field both leave the old arms green. The two OLD-RED-NEW-PASS rows are what fired on commandprompt#1077 and cost a correct PR a red. MY FIRST MUTATION MATRIX WAS WRONG AND I ALMOST SHIPPED IT. It reported the second-keyed-lookup case reddening an unrelated `natts` arm. Run alone that case is 24/24. The harness asserted each mutation APPLIED and never asserted it was RESTORED, so one case was measuring two mutations. Re-run with an md5 restore assertion per case, and the contamination is gone. A mutation harness that does not check its own restore produces exactly the false finding I would have filed. FIVE MAJORS, both suites, own `make clean` each: PG15 PG16 PG17 PG18 PG19 native_fetch_cache PASSED, native_fetch_position PASSED No ledger change. `native_fetch_cache` and `native_fetch_position` have zero rows, so they are two of the 249 uncovered suites and no check name here is a ledger key. Checked, not inherited from commandprompt#1078. Four of the twelve remain, in native_fetch_cache (0 left), decode_interrupts (1, correct), native_saop_pushdown (1, correct) and the two commandprompt#1078 already fixed -- leaving native_fetch_cache's siblings done and nothing outstanding in these two files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs

jdatcmd

Reviewed the two you left alone and the geometry repair, because those are where
the judgement is. Red first, then the substance.

The red is your own new arms, and I reproduced it with sites

FAIL  no suite pipes a captured string into an early-exit reader: got [10] want [0]

Five distinct sites, each counted twice because the sweep reports the logical line
and its physical continuation:

native_fetch_cache.sh:231    printf '%s\n' "$_nfc_slot" | grep -qF 'e->cid != cid'
native_fetch_cache.sh:238    printf '%s\n' "$_nfc_row"  | grep -qE "entry->$_f != ..."
native_fetch_cache.sh:246    awk "/^$_fn\(/,/^}/" ...   | grep -qF 'PgColumnarDiscardFetchCache'
native_fetch_position.sh:241 printf '%s\n' "$_nfp_row"  | grep -qF 'present = ...'
native_fetch_position.sh:245 printf '%s\n' "$_nfp_row"  | grep -qF 'entry->valOffset...'

case "$var" in *needle*) echo yes ;; *) echo no ;; esac is the no-pipe idiom and
is already used at native_fetch_projection.sh:155.

Worth saying the detector is behaving well: the pre-existing | sort -n | head -1
at :100 and :141 are NOT flagged, because sort drains its input before
writing, so head closing early reaches sort rather than the printf.

`native_saop_pushdown`: agreed, and it is already labelled correctly

Verified the coupling you describe. awk '/if $key->sk_flags & \(/,/$\)/' on the
next line restarts on each match, so a second if (key->sk_flags & ( site would
give TWO ranges, and tr -d ' \t\n' would concatenate them into one guard
string. Every membership test then passes on the union. The count is load-bearing
for the awk range, and the arm already says premise: in its own name. Leave it.

`decode_interrupts`: right conclusion, and the REASON does not hold here

^#define COLUMNAR_DECODE_INTERRUPT(i) -- twice is a redefinition

Not in this codebase. Defining a macro twice under version guards is the
established pattern, and src/columnar_compat.h does it 33 times:

#if PG_VERSION_NUM >= 180000
#define COLUMNAR_COLLATION_IS_C(collid) \
	(OidIsValid(collid) && pg_newlocale_from_collation(collid)->collate_is_c)
#else
#define COLUMNAR_COLLATION_IS_C(collid) \
	(OidIsValid(collid) && lc_collate_is_c(collid))

So a second #define is not a redefinition, it is the compatibility idiom, and an
#if-guarded variant would redden that arm exactly the way an honest second caller
reddened the ones we just repaired. Same false-alarm direction, rarer trigger.

I still agree with leaving it: COLUMNAR_DECODE_INTERRUPT wraps
CHECK_FOR_INTERRUPTS, which is stable across all five majors, so a version
variant is not on the horizon. But the reason in the CHANGELOG is the thing the
next person reuses, and "twice is a redefinition" will send them wrong on a macro
in compat.h. The accurate version is narrower: this macro has no
version-dependent form, so one definition is the property.

The geometry repair is right, and I checked the four fields exist

firstRowNumber  1    rowCount  1    fileOffset  1    natts  1

All four are compared in columnar_reader.c, so the loop is asserting four real
things rather than widening one. That is the sharpest of the six, as you said:
counting fileOffset said nothing about the other three, and dropping rowCount
was invisible.

One nitpick, not a blocker. The alternation entry->$_f != rg->$_f|entry->$_f != natts exists because natts compares against a bare natts, but it applies the
second branch to every field. A source line reading entry->rowCount != natts
would satisfy the rowCount arm wrongly. A per-field expected pattern would be
tighter. Harmless today; mentioning it because the whole PR is about arms matching
their names.

On your restore bug

Asserting the mutation APPLIED but not that it was RESTORED is the same family as
my stale .pyc: the way out needs a guard as much as the way in. Worth having in
the CHANGELOG as its own line rather than only in the matrix, since the matrix now
looks clean and the reason it is clean is the fix.

Not approving while it is red. Everything above stands independent of the pipe
fix, so none of it should need re-checking after you push.

@jdatcmd

My own new arms tripped selftest/080: five sites piped a captured string into `grep -q`, which exits on its first match and closes the pipe under its writer. Caught by @jdatcmd on commandprompt#1081's CI, with the sites rather than a count. The answer was already in the file family I was working in -- native_fetch_projection.sh:155 uses a `case` over a captured variable. No subprocess, no pipe, and it reads better than the `&& echo yes || echo no` tail. Patterns are quoted so the brackets in valOffset[c][present] stay literal rather than becoming glob character classes. AND THE DISCARD ARMS GAINED THE PREMISE THEY WERE MISSING. As written they could not tell "this function does not discard the cache" from "the awk range matched nothing" -- the same both-readings problem I raised against commandprompt#1078's literal match, in my own arm one day later. Each function's extraction is now captured, asserted non-empty, and then matched. Re-verified rather than assumed, because the arms changed: selftest/080 no suite pipes a captured string into an early-exit reader PASS suites native_fetch_cache 26 passed, native_fetch_position 8 passed matrix identical verdicts to the pre-fix run, every restore md5-checked five majors PG15 PG16 PG17 PG18 PG19, both suites, PASSED The pre-existing `| sort -n | head -1` sites in the same file are correctly NOT flagged: `sort` reads all its input before writing, so `head` closing early reaches `sort` rather than the producer. The detector discriminates rather than banning pipes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs

jdatcmd

APPROVE at 0c9aace. Green at 14/14, and I broke the fixture rather than reading
the matrix.

The pipe fix is right, and the two that remain are correct

All five sites are case over a captured variable. The two pipes still in
native_fetch_cache.sh are :100 and :141, the pre-existing
| sort -n | head -1, which are correctly not flagged: sort drains its input
before writing, so head closing early reaches sort and not the producer.

I verified your headline independently

Planted the defect the geometry arm exists to catch -- dropped rowCount from the
recheck in columnar_reader.c -- and ran the real suite:

mutation applied   entry->rowCount != rg->rowCount  1 -> 0
suite              25 passed + 1 failed + 0 unrunnable + 0 skipped = 26
the one red        a hit re-checks the group's rowCount against the row group:
                   got [no] want [yes]
restored           git diff empty

Exactly one arm, and it is the right one. The old arm counted fileOffset,
which this mutation leaves untouched, so it would have passed. That is the case
for the whole PR in one run.

The `:246` repair is better than what I asked for

I suggested capturing and asserting non-empty. You generalised it into a loop over
both functions with premise: $_fn was extracted, not an empty range per
function, so a renamed pgcolumnar_xact_callback reddens as a premise rather than
as a missing call. That is the distinction the arm could not previously make.

One thing NOT addressed, and I am approving anyway

The CHANGELOG still reads:

decode_interrupts.sh's ^#define COLUMNAR_DECODE_INTERRUPT(i) would be a
redefinition if it appeared twice

True of that macro -- I checked, columnar_encoding.c:161 carries no #if
guard, so a second unguarded definition would be a redefinition. My objection is
about how it will be READ, not whether it is true here: defining a macro twice
under version guards is this codebase's standard compatibility idiom, and
src/columnar_compat.h does it 33 times. Someone reusing "twice is a
redefinition" as a principle on a compat.h macro writes a broken arm.

Not blocking on it. It is prose about a reason, the code is complete and correct,
and the narrower phrasing -- this macro has no version-dependent form -- can
land here or in the next sweep. Your call.

And one nitpick I raised that is still open, also not blocking

The geometry loop's alternation *"entry->$_f != rg->$_f"*|*"entry->$_f != natts"*
applies the second branch to every field, so a source line reading
entry->rowCount != natts would satisfy the rowCount arm. Harmless today: all
four fields are present in the correct form, which I checked. Mentioning it only
because the PR's whole subject is arms matching their names.

Neither of those is worth holding six correct repairs for.

The only conflict is CHANGELOG.md, and both sides add entries to `### Fixed`, so both are kept. #1080, #1081 and #1083 landed while this was open. Verified by count rather than by reading the diff: markers left 0 entries present exactly once #1074/#1076, #1075, #1077, #1080, #1081 bodiless headings in [Unreleased] 0 Nothing else moved. Per-file patch md5 of my seven files, merged result against the pre-merge branch, added and removed lines only: docs/administration.md 6a456b0edcac same docs/best-practices.md 0c84c03e77bd same docs/configuration.md 68b2705ebe8a same test/fsst_margin.sh 978e4e448429 same test/pytest/TESTS.md 787346d68405 same test/pytest/expected_tests.txt d1807c9dfd54 same test/pytest/test_compression_reaches_the_cascade.py 025da1ba7426 same main moved none of `expected_tests.txt`, `check_ledger.tsv` or `check_ledger_budget.txt` -- checked by md5 against 8e88f42 rather than assumed from the fact that the merges were docs, shell suites and one `src/` file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK

CHANGELOG.md only. #1081 and #1083 landed while this was open and both add to `### Fixed`, as this does, so all three entries are kept. Verified by count rather than by reading the diff: conflict markers left 0 each entry present exactly once #1077 sweep, #1075, #1080, #1081, and #1078's, which was already there bodiless headings in [Unreleased] 0 The suite file is untouched by the merge: its patch md5 against main is unchanged from before it, and the 17 check names are identical by sorted diff, so no ledger key moves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK

@jdatcmd

#1081 replaced a count of `entry->fileOffset != rg->fileOffset` with a per-field membership loop, so dropping any one of the four compared fields reddens by name. The loop matched `entry->$_f != rg->$_f` OR `entry->$_f != natts` for EVERY field, because `natts` is the one compared against the scan's own column count rather than against the row group. Harmless today -- `entry->firstRowNumber != natts` appears nowhere -- and still wrong as a claim: each arm would accept a comparison its own name denies. Three fields now match only the `rg` form; `natts` has its own arm and its own name. Reported by @jdatcmd reviewing #1081. ALL FOUR VERIFIED BY REMOVAL on the real suite, each reddening only its own arm: firstRowNumber removed FAIL ... firstRowNumber against the row group rowCount removed FAIL ... rowCount against the row group fileOffset removed FAIL ... fileOffset against the row group natts removed FAIL ... natts against the scan's column count restored 26 passed, source byte-identical FIRSTROWNUMBER IS THE ONE #1081 SHIPPED UNPROVEN, and the reason is worth keeping. Its mutation never applied: that line begins `(entry->` rather than `entry->`, so the pattern missed, and the harness's applied-assertion REPORTED it instead of letting a clean run count as a pass. A mutation that does not apply looks exactly like an arm that does not fire. Five majors: PG15 PG16 PG17 PG18 PG19, native_fetch_cache PASSED. No ledger change: one check name is added and none removed, and the suite has zero rows in the ledger -- it is one of the 249 uncovered suites. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs

…ison-is-not-against-rg The natts comparison is not against the row group (#1081)

jdatcmd reviewed Sep 16, 2026

View reviewed changes

jdatcmd approved these changes Sep 16, 2026

View reviewed changes

jdatcmd merged commit 410e06b into commandprompt:main Sep 16, 2026
14 checks passed

jdatcmd mentioned this pull request Sep 16, 2026

docs: pgcolumnar.compression picks encodings, not only a codec (#1074, #1076) #1082

Merged

jdatcmd mentioned this pull request Sep 16, 2026

test: the decode arms could not see a decode (#1077 sweep) #1079

Merged

OffgridwithJD mentioned this pull request Sep 16, 2026

The natts comparison is not against the row group (#1081) #1084

Merged

jdatcmd added a commit that referenced this pull request Sep 16, 2026

Merge pull request #1084 from OffgridwithJD/fix/1084-the-natts-compar…

d959525

…ison-is-not-against-rg The natts comparison is not against the row group (#1081)

OffgridwithJD mentioned this pull request Sep 16, 2026

release: META.json named a script the distribution does not contain #1086

Merged

Navigation Menu

Pin six more guards to their property, not their caller count (#1078) - #1081

Pin six more guards to their property, not their caller count (#1078)#1081
jdatcmd merged 2 commits into
commandprompt:mainfrom
OffgridwithJD:fix/1079-pin-the-property-not-the-caller-count

OffgridwithJD commented Sep 16, 2026

Uh oh!

jdatcmd left a comment

Uh oh!

jdatcmd left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

OffgridwithJD commented Sep 16, 2026

The repair differs per arm, because the failure direction does

Two of the eight I was assigned are correct and are left alone

Measured

My first mutation matrix was wrong and I nearly shipped it

Five majors, both suites, own make clean each

No ledger change

What remains of the twelve

Uh oh!

jdatcmd left a comment

Choose a reason for hiding this comment

The red is your own new arms, and I reproduced it with sites

native_saop_pushdown: agreed, and it is already labelled correctly

decode_interrupts: right conclusion, and the REASON does not hold here

The geometry repair is right, and I checked the four fields exist

On your restore bug

Uh oh!

jdatcmd left a comment

Choose a reason for hiding this comment

The pipe fix is right, and the two that remain are correct

I verified your headline independently

The :246 repair is better than what I asked for

One thing NOT addressed, and I am approving anyway

And one nitpick I raised that is still open, also not blocking

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Five majors, both suites, own `make clean` each

`native_saop_pushdown`: agreed, and it is already labelled correctly

`decode_interrupts`: right conclusion, and the REASON does not hold here

The `:246` repair is better than what I asked for